fix handling of null groupConfiguration#1130
Conversation
Fixes aws#1129 . I added the "nullish" serde function and test emulating similar treatment for nullish boolean and map fields. i think the new deserialize_nullish function could supersede deserialize_nullish_boolean, deserialize_lambda_map, and possibly deserialize_lambda_dynamodb_item. but i left that out for a potential follow-up PR, to keep the blast radius small.
jlizen
left a comment
jlizen
left a comment
There was a problem hiding this comment.
Looks reasonable, just some small doc tweaks
| #[serde(deserialize_with = "deserialize_lambda_map")] | ||
| #[serde(default)] | ||
| pub user_attributes: HashMap<String, String>, | ||
| #[serde(default, deserialize_with = "deserialize_nullish")] |
There was a problem hiding this comment.
Can we give this field a doc comment mentioning the "null-ish as default" semantics?
| #[serde(deserialize_with = "deserialize_lambda_map")] | ||
| #[serde(default)] | ||
| pub user_attributes: HashMap<String, String>, | ||
| #[serde(default, deserialize_with = "deserialize_nullish")] |
There was a problem hiding this comment.
Can we give this field a doc comment mentioning the "null-ish as default" semantics?
There was a problem hiding this comment.
will do (and the other occurrence)
addressed review feedback for aws#1129 . all of the implementations for the removed functions were identical, so they are exactly equivalent to the generic deserialize_nullish. existing unit tests demonstrate the equivalence. functions are private to the crate and safe to remove. I added one assertion which demonstrates failing behavior for missing fields (which are *not* covered by this deserializer). the behavior for that case is the same before and after the change, it just wasn't tested. also updated field documents and the helper function to explain the semantics.
jlizen
left a comment
jlizen
left a comment
There was a problem hiding this comment.
The changes look great, thanks for this @jct-tympanon !
Can we have at least a couple fixtures capturing the migrated nullish handling though? (IE, missing the values). The existing fixtures already have contents.
sure will do. i take it "fixtures" here means the sample documents like: https://github.com/aws/aws-lambda-rust-runtime/blob/main/lambda-events/src/fixtures/example-cognito-event-userpools-pretokengen-v2.json , plus test cases which read them. |
|
Yep, exactly! Much appreciated. |
Following CR feedback on aws#1130 . The selected cases cover booleans, dynamodb items, maps, and the GroupConfiguration data structure, so we have at least one instance of each nullish use case. I added a utility function for basic serde verification. I retrofitted it to existing tests for the data structures that got new cases, just so the code is consistent for them. There are a very large number of other similar cases that I left alone to keep the review reasonable.
jlizen
left a comment
jlizen
left a comment
There was a problem hiding this comment.
Looks great, thanks for handling all those other cases!
I'll start the release process today or tomorrow.
2 participants
Fixes #1129 .
📬 Issue #, if available: 1129
✍️ Description of changes: I added the "nullish" serde function and test emulating similar treatment for nullish boolean and map fields. this addresses the problem with the groupConfiguration field discussed in the issue report.
i think the new deserialize_nullish function could supersede deserialize_nullish_boolean, deserialize_lambda_map, and possibly deserialize_lambda_dynamodb_item. but i left that out for a potential follow-up PR, to keep the blast radius small.
🔏 By submitting this pull request
cargo +nightly fmt.cargo clippy --fix.